Just wanted to point out that the spec technically allows for other type parameters:

  message Parameter {
    oneof parameter {
      // Explicitly null/unspecified parameter, to select the default value (if
      // any).
      google.protobuf.Empty null = 1;

      // Data type parameters, like the i32 in LIST<i32>.
      Type data_type = 2;

      // Value parameters, like the 10 in VARCHAR<10>.
      bool boolean = 3;
      int64 integer = 4;
      string enum = 5;
      string string = 6;
    }
  }

That being said, I would prefer you leave what you have as is since the PR is fairly large already. Just a note for future work!

Is there a reason to separate out builtin parameterized types which don't take in type parameters from those that do? Not saying this is necessarily wrong, just curious to understand why. Substrait-java treats both e.g. decimal (which takes only an int param) and list as implementors of BaseParameterizedType.

Thank you for these wonderful comments 🙏

Any reason not to consistently use Type here? I.e. BuiltinType.

Just a note (haven't checked yet). It would be good to make sure there is a test specifically for structure.

This is kind of a rarer feature which in which we only recently introduced handling for substrait-go and is still a WIP for substrait-java.

Ah there kind of implicitly is a test for this because of the extension_types.yaml file in the core extensions, which the registry.rs at least checks is able to load everything without an error.

Just an idea, and doesn't necessarily have to be this PR, but what do you think about just making the Context and Parse traits pub(crate)? As I understand it, the only public API necessary for parsing are just the impls for ExtensionFile.

I don't see this function used anywhere yet. Wondering if it makes sense to just switch the return type to be Option<&Urn>? I could imagine contexts in which we might want to use this function but not consider the "not found" case an error.

nit: Does parsing Version require ExtensionAnchors context, or any context at all? Could imagine either passing around () for empty context, or having two traits, one called something like Parse and another called ParseWith.

I'm just dumping some thoughts though :) this doesn't have to be in this PR, especially considering that Parse already existed.

I haven't checked myself yet if it already exists but just noting that it would be good to have tests somewhere that ensure all of the core extension files parse correctly. Could be just as simple as showing they don't result in an error.

Yup, you already got it in the registry file, nice!